Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support !Sync (and maybe !Send) Context #840

Closed
wants to merge 4 commits into from
Closed

Support !Sync (and maybe !Send) Context #840

wants to merge 4 commits into from

Conversation

AzureMarker
Copy link

@AzureMarker AzureMarker commented Jan 1, 2021 •
edited
Loading

When upgrading juniper from 0.9 to 0.15, I found that context types now require Send + Sync. Previously, before juniper was async, everything was synchronous and the context could contain !Sync types such as database connections (ex. SQLite). This PR experiments with removing the Sync requirement (and Send, since the context is only ever referenced not owned).

There are some roadblocks which I'm not sure can be resolved at this time. The juniper futures now are !Send since they hold a reference to the !Sync context. This is fine in the core juniper library and juniper_actix, but web services based on tokio/hyper have issues with spawning !Send futures. Tokio supports spawning !Send futures, but only when using a LocalSet, which is only really feasible when using the single-threaded runtime. There is a (recently closed, but in-discussion) issue which addresses spawning !Send futures on the multi-threaded runtime: tokio-rs/tokio#2545. Due to this issue, Hyper only supports !Send futures when using the single-threaded runtime: https://github.com/hyperium/hyper/blob/8861f9a7867216c81ea14ac6224c11a1303e7761/examples/single_threaded.rs. Warp does not support !Send yet: seanmonstar/warp#528. It's not clear how Rocket 0.5 will handle !Send, but since it's based on hyper it won't be any better than what they provide.

Theoretically the future that runs the graphql resolver could be Send because the !Sync context is owned by the future and passed into juniper by reference. However, because GATs (Generic Abstract Types) are not implemented, traits with async methods must box their futures. This causes juniper's future to hold LocalBoxFutures, which are not Send. Due to this, the overall future is not Send... If GATs were implemented, it may be possible to avoid the tokio !Send futures issue by convincing Rust that the overall juniper resolve future is Send since the context is owned by the calling code (the web handler which calls juniper) which is part of the same future object.

The main changes include:

  • Removing Send and Sync bounds on context types.
  • Replacing BoxFuture with LocalBoxFuture (similar for streams), due to the removal of Context: Sync.
  • Updating juniper_hyper examples and tests to use a single-threaded runtime due to the above issues.
  • Updating juniper_warp in an attempt to support !Send futures, but this fails at runtime due to the lack of support in warp for !Send futures (see issue link above).
  • Move the config out of ExecutionParams so that the context is not captured in the error type, which would require the context to be Send. This is safe because the error object only refers to data in the request and schema, not the context (see the lifetimes on juniper::execute).

I don't really expect this PR to be merged any time soon, if at all, but it's worth bringing up the issue and showing how the core juniper library can support !Sync (and even !Send) context. The issues lie in the web server integration libraries, mainly due to the lack of GATs and issues in tokio with spawning !Send futures.

Related juniper issues:

@AzureMarker AzureMarker mentioned this pull request Jan 1, 2021
Closed
AzureMarker added a commit to forte-music/core that referenced this pull request Jan 2, 2021
@AzureMarker
Update the rest of the dependencies and code to modern versions (async)
23e7400 
Notably updated actix, tokio, and juniper.

Juniper now requires Send + Sync for context, prompting me to open
graphql-rust/juniper#840

The file streaming code has been replaced by the ones provided by actix,
as they support everything we need now.

Signed-off-by: Mcat12 <[email protected]>
@tyranron
Copy link
Member

tyranron commented Jan 4, 2021

@Mcat12 so this PR makes juniper usable only with single-threaded runtimes, right? If so, I'm very unsure this chage worth such an impact. Maybe it's better to use send_wrapper where you don't care about synchronization as where stated here: #828 (comment)?

@AzureMarker
Copy link
Author

AzureMarker commented Jan 4, 2021 •
edited
Loading

@Mcat12 so this PR makes juniper usable only with single-threaded runtimes, right?

Not quite, or at least that's not the goal. I noted some of the current roadblocks in detail in the description, but basically juniper should be able to support !Sync context while still running on a multi-threaded runtime. My goal is to have the future returned by juniper::execute be Send if the context is Sync (keeping the current behavior), and be !Send only if the context is !Sync.

Juniper's core machinery is synchronous, operating within one future chain. The asynchrony comes from the user's resolver code, which can be async functions. This synchronous behavior supports having !Sync context, but proving that to the compiler right now is difficult.

The PR description notes some reasons why that is not the case currently, most notably that async functions cannot appear in traits due to a lack of GATs (Generic Associated Types). See for example GraphQLValueAsync::resolve_async. Another issue is that it is hard to spawn !Send futures in the multi-threaded tokio runtime. I've opened a PR to add a spawn_pinned API which would fix this shortcoming, but this limitation is why the hyper and warp juniper crates have issues in this PR. These roadblocks are being worked on, so eventually this idea should work out.

If you would like, I can mark this PR as a draft since it is unlikely to be merge-able in the short term.

Edit: Regarding send_wrapper, that only works for actix (and other single-threaded runtimes). It doesn't fix the case of hyper and warp.

@tyranron
Copy link
Member

tyranron commented Jan 4, 2021

@Mcat12 the biggest blocker is GAT, of course. The fact that juniper cannot be Send/Sync transparent is due to trait objects usage in traits. I've played with this multiple times last year and haven't figured out how it can be reasonable managed without GATs.

Things like whether runtime supports !Send futures or not are not quite juniper related, as it doesn't use runtimes directly. So, the best option is to be Send/Sync transparent.

If you would like, I can mark this PR as a draft since it is unlikely to be merge-able in the short term.

Yeah, that would be nice.

Edit: Regarding send_wrapper, that only works for actix (and other single-threaded runtimes). It doesn't fix the case of hyper and warp.

And it shouldn't, as for me. Running !Send/!Sync contexts on multi-threaded runtimes with work stealing should be prevented by compiler.

@AzureMarker AzureMarker marked this pull request as draft January 4, 2021 18:58
@AzureMarker
Copy link
Author

Edit: Regarding send_wrapper, that only works for actix (and other single-threaded runtimes). It doesn't fix the case of hyper and warp.

And it shouldn't, as for me. Running !Send/!Sync contexts on multi-threaded runtimes with work stealing should be prevented by compiler.

Although it's somewhat unrelated to the core juniper library, this is where spawn_pinned in tokio comes in. This API would allow libraries like warp (which do not support !Send futures) to support !Sync context, making all of the supported web servers able to run with !Sync context.

@LegNeato LegNeato mentioned this pull request Jul 24, 2021
Open
@AzureMarker
Copy link
Author

Closing this PR since any future progress will require starting from scratch.

@tyranron tyranron added enhancement Improvement of existing features or bugfix wontfix This will not be worked on labels Jan 30, 2023
@tyranron tyranron added this to the 0.16.0 milestone Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@AzureMarker AzureMarker

Labels
enhancement Improvement of existing features or bugfix wontfix This will not be worked on
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
2 participants
@AzureMarker @tyranron